-
Notifications
You must be signed in to change notification settings - Fork 3
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[POC] Emotion Themes #387
[POC] Emotion Themes #387
Conversation
…COLORS functionality
} | ||
`; | ||
|
||
export const AccordionBox = styled.div<{ | ||
noBorder: boolean; | ||
isOpen: boolean; | ||
disabled: boolean; | ||
theme?: ThemeType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Emotion 10 recommends exporting a typed styled
file to allow us to pass the Theme into styled.div
implicitly: https://emotion.sh/docs/typescript
However, this breaks our jest
tests when wrapped with a ThemeProvider
. Emotion 11 will introduce more configuration to handle this more gracefully (docs, issue comment) but has not yet been released.
Apparently Emotion 11 is supposed to be stable, though, despite no release, so that is an option to consider if the verbosity is painful.
@@ -0,0 +1,60 @@ | |||
import PropTypes from 'prop-types'; | |||
|
|||
import COLORS from '.'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
src/constants/colors/index.ts
is unchanged to keep the diff to a minimum, but introducing full theming support would require making changes to that file as well. We'd want to disallow access to COLORS
directly except under specific circumstances.
I think our goal should be to make opting-in to the Theme as easy as possible. Personally I've already enjoyed not needing to import COLORS
and just access it (and already typed!) off of a theme.
Closing in favor of #466 |
We will be incorporating a second theme in
radiance-ui
shortly. This PR is a POC for the functionality. It pulls in only theToggle
component story and adds very minimal theming support for it, as well as a button toggle in the story to swap between the themes.This POC PR does not explore Storybook add-ons that help you easily toggle between themes. You can do a search for "theme" in the Community Add-ons section of Storybook (link) but none seem to have all too much usage, so perhaps better to not add the dependency debt. Dealing with how to best document our themes is something we can figure out later.
In the meantime, I've just added a button to the top of the story to switch themes.
The review app can be found here: https://curology-radiance-pr-387.herokuapp.com/?path=/story/*